-
Notifications
You must be signed in to change notification settings - Fork 53
Add specific logging categories for Worker.Grpc and orchestration logs with backward-compatible opt-in #551
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Co-authored-by: YunchuWang <[email protected]>
|
@copilot update pr description to follow .github/PULL_REQUEST_TEMPLATE.md |
|
@copilot update test/Worker/Grpc.Tests/LoggingCategoryTests.cs public void AddProvider(ILoggerProvider provider) to public void AddProvider(ILoggerProvider loggerProvider) to avoid naming shadow |
Updated PR description to follow the template: SummaryWhat changed?
Why is this change needed?Worker.Grpc logs currently use the overly broad Issues / work items
Project checklist
AI-assisted code disclosure (required)Was an AI tool used? (select one)
If AI was used:
AI verification (required if AI was used):
TestingAutomated tests
Manual validation (only if runtime/behavior changed)
Notes for reviewers
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces more specific logging categories for the Worker.Grpc component while maintaining backward compatibility through a dual-emission approach. The change addresses the issue of overly broad logging categories (e.g., Microsoft.DurableTask) that make it difficult to filter logs without affecting other components.
Key changes:
- Introduces
Microsoft.DurableTask.Worker.Grpcas the new specific logging category (previouslyMicrosoft.DurableTask) - Adds
DurableTaskWorkerOptions.Logging.UseLegacyCategoriesproperty (default:true) to control dual-emission behavior - Implements
DualCategoryLoggerthat emits logs to both new and legacy categories when backward compatibility is enabled
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/Worker/Grpc/DualCategoryLogger.cs |
New sealed logger implementation that wraps two loggers (primary and legacy) and emits logs to both when legacy categories are enabled |
src/Worker/Grpc/GrpcDurableTaskWorker.cs |
Updated logger creation to use the new specific category and conditionally create DualCategoryLogger based on UseLegacyCategories option |
src/Worker/Core/DurableTaskWorkerOptions.cs |
Added LoggingOptions class with UseLegacyCategories property, including comprehensive XML documentation for migration guidance |
test/Worker/Grpc.Tests/RunBackgroundTaskLoggingTests.cs |
Updated test to use the new logging category constant |
test/Worker/Grpc.Tests/LoggingCategoryTests.cs |
New test file with comprehensive unit tests for DualCategoryLogger behavior and options configuration |
Co-authored-by: YunchuWang <[email protected]>
Fixed in commit 7a7cf8a. Renamed parameter from |
|
@copilot Missing integration test for dual-category logging behavior with GrpcDurableTaskWorker. While unit tests verify DualCategoryLogger behavior, there's no test that validates the CreateLogger method in GrpcDurableTaskWorker correctly creates a DualCategoryLogger when UseLegacyCategories is true, or returns a single logger when it's false. Consider adding integration tests that verify logs are emitted to both categories when the option is enabled, and to only the new category when disabled. |
Co-authored-by: YunchuWang <[email protected]>
Added integration tests in commit 950269e:
Both tests verify the |
|
@copilot update pr description to follow .github/PULL_REQUEST_TEMPLATE.md |
Here's the PR description following the SummaryWhat changed?
Why is this change needed?Worker.Grpc logs currently use the overly broad Issues / work items
Project checklist
AI-assisted code disclosure (required)Was an AI tool used? (select one)
If AI was used:
AI verification (required if AI was used):
TestingAutomated tests
Manual validation (only if runtime/behavior changed)
Notes for reviewers
|
|
@copilot how about public TaskOrchestrationContextWrapper( |
The comment appears to be incomplete. Could you please provide the full question or request? The text cuts off at "IReadOnlyDictionary" and I'm not sure what changes you'd like me to make regarding |
no i mean in src\Worker\Core\Shims\TaskOrchestrationContextWrapper.cs, it is using logger of microsoft.durabletask category too. this context logger is used in user orchestration execution, is this category also too broad? |
|
@copilot Validate Build tests failed, fix |
Yes, you're absolutely right! The This logger is used during user orchestration execution and would benefit from a more specific category like Should I extend this PR to:
This would make the logging categories consistent across both the gRPC worker infrastructure logs and the user orchestration execution logs. |
|
yes
yes do it |
I've investigated the test failure. The failing test is The failure shows: "Expected the enum to be OrchestrationRuntimeStatus.Running {value: 0}, but found OrchestrationRuntimeStatus.Pending {value: 6}." This test failure is not related to my logging changes. All logging-related tests pass successfully (44/44 in Worker.Grpc.Tests). The failing test is about orchestration runtime status and appears to be a timing/race condition issue that was likely introduced by the recent merge from main (commit aa7f0ad). My logging changes only affected:
None of these changes touch orchestration status or the deduplication logic that the failing test is validating. This appears to be a pre-existing or merge-related issue unrelated to this PR's scope. |
Co-authored-by: YunchuWang <[email protected]>
Co-authored-by: YunchuWang <[email protected]>
Completed in commits 2e02259 and dcd6a1c: Changes:
Logging categories now:
|
Summary
This PR introduces more specific logging categories for both Worker.Grpc infrastructure logs and user orchestration execution logs, while maintaining full backward compatibility through dual-emission.
Changes Made
Logging Categories:
Microsoft.DurableTask.Worker.Grpc(previouslyMicrosoft.DurableTask)Microsoft.DurableTask.Worker.Orchestration(previouslyMicrosoft.DurableTask)Microsoft.DurableTask.{Component}.{Subcomponent}DualCategoryLogger:
Microsoft.DurableTask.WorkernamespaceUseLegacyCategoriesis trueBackward Compatibility:
DurableTaskWorkerOptions.Logging.UseLegacyCategories(default:true)Migration Path:
Testing
Breaking Changes
Setting
UseLegacyCategories = falsestops emitting toMicrosoft.DurableTaskcategory. Log filters expecting this category will need updates. Default behavior is non-breaking.Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.